Skip to content

C++: reduce scanf false positives after fallback initialization - #22804

Open
SkyCipherr wants to merge 1 commit into
github:mainfrom
SkyCipherr:codex/fix-missing-check-scanf
Open

SkyCipherr wants to merge 1 commit into
github:mainfrom
SkyCipherr:codex/fix-missing-check-scanf

Conversation

@SkyCipherr

Copy link
Copy Markdown

Summary

Related to #12412.

The cpp/missing-check-scanf query can report an output variable after a failed scanf-like call even when the failure branch assigns a default value. The successful flow still reaches the merged read, where the existing guard check does not recognize that both branches have initialized the variable.

This change adds an indirect IR guard barrier for branches that guarantee the corresponding output was written. It reuses the existing minimum conversion count, including the handling of suppressed conversions and %n, and retains the existing guard check at the read. Unsafe reads on the failure path remain tracked.

The barrier is restricted to non-reference local output variables whose other lvalue uses are assignments, increments, or scanf output arguments. Other address uses and reference outputs are excluded because alias reads are not fully modeled by the current flow configuration.

Tests

  • Add 19 C and 5 C++ independently written regression functions covering fallback initialization, saved and overwritten return values, multiple outputs, partial initialization, conversion counts, conditional reads, unsafe updates, repeated calls, and pointer/reference aliases.
  • Remove the two existing known false positives in scan_and_write and regenerate the expected results with codeql test accept after reviewing the differences.
  • Retain two known safe-alias false positives as SPURIOUS assertions. Existing alias-read modeling gaps remain marked as MISSING, with assertions preserving the downstream warnings.
  • Add a minorAnalysis change note.

Validation used CodeQL CLI 2.27.2 against main at b5b81654b6e16786e6c5669f696d423d8764e336:

codeql query format --check-only cpp/ql/src/Critical/MissingCheckScanf.ql
codeql test run cpp/ql/test/query-tests/Critical/MissingCheckScanf --additional-packs=. --threads=2 --ram=4096
codeql test run cpp/ql/test/library-tests/scanf --additional-packs=. --threads=2 --ram=4096
python misc/scripts/check-query-ids.py
git diff --check

Both query tests and all three library tests passed. The query ID check passed with Python 3.12.14. The added fixtures also passed GCC C11 and G++ C++11 syntax-only checks.

A comparison of the unchanged main query and this candidate on the same expanded test database produced 52 and 35 results respectively: 17 removed and none added. Of the removed results, 14 are safe-case false positives and 3 are redundant later uses after an unsafe read that is still reported. The original test.cpp results changed only from 21 to 19 by removing its two known false positives.

A separate minimal C/C++ database, extracted through real GCC/G++ compilation, was analyzed with --rerun: its unsafe read remains reported, while its safe fallback read is no longer reported (two alerts become one).

The complete original gethostid.c file linked in the issue was also compiled and extracted separately, with its contents verified against Git blob fcef8798c615c55e62c5b272b36c2cc241d4e134. On the same database, the unchanged query reports the return hostid at line 55; this candidate does not. An unchecked-read control remains reported, a safe fallback control becomes unreported, and a zero-initialized static-local control is unreported by both queries. This was a Windows/MinGW extraction with a local platform-compatibility header for sys/systeminfo.h, not a full OpenZFS build. The target function's source was unchanged. None of this third-party source is included in the proposed changes.

Scope and Limitations

This is not a general pointer or reference aliasing fix. Safe fallback cases with other address uses may still be reported, and pre-existing alias-read modeling gaps remain. The results above describe the covered fixtures, not a proof of absence of false negatives in other programs.

The historical static-initialization concern mentioned in the issue is already unreported in the tested main baseline; this change does not claim credit for that behavior. Other historical code-scanning alerts linked in the discussion have not been individually reproduced from their complete projects.

The full repository test suite and a full OpenZFS build were not run. Upstream CI and the change-note release workflow have not been run locally.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant